Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added conditional checks for empty grid placement #2640

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Sahil-Chhoker
Copy link
Contributor

Summary

Added if checks to confirm existence of agents before trying to place them on the grid, which before resulted in a error, now just displays the empty grid.

Bug / Issue

fixes #2638

Additional Notes

No changes made to the draw_hex_grid function since the required changes are included in #2609.

Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔵 -0.4% [-1.0%, +0.1%] 🔵 -0.8% [-1.0%, -0.6%]
BoltzmannWealth large 🔵 +1.1% [+0.7%, +1.6%] 🔵 +0.4% [+0.0%, +0.8%]
Schelling small 🔵 +0.3% [+0.1%, +0.5%] 🔵 +0.6% [+0.4%, +0.7%]
Schelling large 🔵 +0.4% [-0.1%, +0.8%] 🔵 -0.1% [-0.7%, +0.5%]
WolfSheep small 🔵 +0.3% [+0.1%, +0.6%] 🔵 -0.3% [-0.5%, -0.2%]
WolfSheep large 🔵 +0.7% [+0.3%, +1.1%] 🔵 +0.3% [-0.2%, +0.7%]
BoidFlockers small 🔵 -0.9% [-1.5%, -0.4%] 🔵 -1.0% [-1.3%, -0.7%]
BoidFlockers large 🔵 -1.5% [-2.1%, -1.0%] 🔵 -1.3% [-1.6%, -1.0%]

@quaquel
Copy link
Member

quaquel commented Jan 26, 2025

This is related to #2638 and #2642.

I need to think about the best way forward to address this. Yes, we can add a bunch of if statements as done here. However, I am inclined to see this as an architectural problem. At the moment, we don't distinguish carefully between drawing the space (e.g., the grid structure), the drawing of properties of the space (i.e., property layer portrayal), and the drawing of agents within the space. These are effectively all collapsed into a single draw_space method. I am inclined to redesign the API and explicitly separate these three things slightly. If you pass the same axes (in the case of matplotlib), you can draw them on top of each other, but if you pass separate axes, you can also portray them more cleanly. Any thoughts on this are welcome, preferably in #2642.

@Sahil-Chhoker
Copy link
Contributor Author

I think we should implement an AgentPortrayalStyle class as proposed in #2436 first. Then we can do something similar with the property layer portrayal as well:

class PropertyLayerPortrayal:
    def __init__(self, layer_name, colormap=None, color=None, alpha=1.0, vmin=None, vmax=None):
        self.layer_name = layer_name
        self.colormap = colormap
        self.color = color
        # etc...

And then we can separate the space drawing into three parts. Maybe something like this:

class SpaceRenderer:
    def draw_structure(self, space, ax=None, **kwargs):
       ...
    def draw_properties(self, space, propertylayer_portrayal, ax=None, **kwargs):
       ...
    def draw_agents(self, space, agent_portrayal, ax=None, **kwargs):
       ...

    # Composite method(not the best name)
    def draw_all(self, space, agent_portrayal=None, property_config=None, ax=None, **kwargs):
       ...

But this design may feel complex for beginners while also breaking the current API.

@quaquel
Copy link
Member

quaquel commented Feb 3, 2025

@Sahil-Chhoker I like the direction of thought: encapsulate the portrayal of agents, space, and propertylayers into their own class, have three dedicated draw functions for each of them, and an overarching one that calls each of them. I'll try to find time to hash this out a bit further in the coming days.

But this design may feel complex for beginners while also breaking the current API.

  1. I don't know yet whether it will be complex. Ideally it should be simple for default cases yet be powerfull enough to cover whatever you want.
  2. the current API is experimental, so we don't have to worry about it too much.
  3. As shown in ADDS ALTAIR PLOT COMPONENT AND SOME OTHER FIXES  #2644, its not too difficult to maintain the current API while raising warnings pointing the user to the new API. This should ease transition.

@Sahil-Chhoker
Copy link
Contributor Author

@quaquel, As I'm expecting, does the implementation look like this in your mind?

@quaquel
Copy link
Member

quaquel commented Feb 3, 2025

The space drawer idea you link to was heavily inspired by altair's API. I still like the idea, and I think it is worth exploring further. However, it is also a more radical departure from the current API. Having <class>PortrayalStyle objects might be a simpler solution that is more compatible with the current design, while still eliminating the difficult to memorize dicts and nested dicts. In short, I am not sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

draw_space only works if there is at least one agent
2 participants